Adding multi chats panel in @jupyter/chat package
#262
Conversation
|
|
|
Thanks @nakul-py for updating it. |
|
bot please update snapshots |
There was a problem hiding this comment.
Thanks again @nakul-py for working on this, this is not an easy change.
This is not a full review.
My main comment is that there shouldn't be any notion of file/path in the generic multi-chat panel (@jupyter/chat) and section, this is only relevant for the jupyterlab-chat widget.
That said, I don't have a clear view of the best way to create a multi-chat panel in jupyterlab-chat:
- we can create a
MultiChatPanelwith functions in options to create, rename, open (your current implementation) - we can have a new class in
jupyterlab-chatthat inherits fromMultiChatPanel, and overrides some of its methods
If you prefer, I can also take it from now, and push in your PR.
| } | ||
|
|
||
| const contentsManager = new ContentsManager(); | ||
| await contentsManager.rename(oldPath, newPath); |
There was a problem hiding this comment.
There shouldn't be any reference to path in this widget.
The notion of file/path is relevant only for jupyterlab-chat, which uses collaborative documents as chat.
| widget: ChatWidget; | ||
| path: string; | ||
| defaultDirectory: string; | ||
| openChat: (path: string) => void; |
There was a problem hiding this comment.
This is not used in the ChatSection I think, only in the panel.
There was a problem hiding this comment.
openChat is used by moveToMain button.
But moveToMain is actually not used anywhere inside ChatSection, so we should remove it
packages/jupyter-chat/src/model.ts
Outdated
| /** | ||
| * Promise that resolves when the model is ready. | ||
| */ | ||
| readonly ready: Promise<void>; |
There was a problem hiding this comment.
| readonly ready: Promise<void>; | |
| get ready(): Promise<void> { | |
| return this._readyDelegate.promise; | |
| } |
We could use a getter here.
packages/jupyter-chat/src/model.ts
Outdated
| @@ -677,6 +698,7 @@ export abstract class AbstractChatModel implements IChatModel { | |||
| private _id: string | undefined; | |||
| private _name: string = ''; | |||
| private _config: IConfig; | |||
| private _readyDelegate: PromiseDelegate<void>; | |||
There was a problem hiding this comment.
| private _readyDelegate: PromiseDelegate<void>; | |
| private _readyDelegate = new PromiseDelegate<void>(); |
Not sure it needs to be set in the constructor.
There was a problem hiding this comment.
Yes, the constructor doesn’t need to do anything for it.
There was a problem hiding this comment.
This widget should probably be located in src/widgets for consistency.
Ok @brichet you can also work on this pr and now renaming, opening, closing and movetomain functions all are working fine. |
…t from the plugin
… to move the chat in the main area
Adding multi chats pannel to
@jypyter/chatpackage and removing it fromjyputerlab-chat. Now it can be easily imported and usable injuputerlab-chatandjupyterlite/ai.